Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix #17383: json.%,to and jsonutils.formJson,toJson now works with uint|uint64 #17389

Merged
merged 3 commits into from
Mar 16, 2021

Conversation

timotheecour
Copy link
Member

fix #17383

@timotheecour timotheecour force-pushed the pr_fix_17383_json_uint64 branch 2 times, most recently from 9e8773e to f2b84db Compare March 15, 2021 23:42
@timotheecour timotheecour force-pushed the pr_fix_17383_json_uint64 branch from a56c5bd to f6d5b39 Compare March 16, 2021 07:51
@timotheecour timotheecour requested a review from ringabout March 16, 2021 08:57
@arnetheduck
Copy link
Contributor

this should solve the most critical issue, though it's worth noting that it breaks backwards compatibility with earlier nim versions.

an aside:

  • why is the isUnquoted flag not inside the JString case?
  • why is it not public? this is significant information when parsing some json files.

in general, json is hard to use because it's an odd juxtaposition of the json standard and nim - in json, there are no integers, only numbers which makes JInt vs JFloat distinction unnatural to work with, from a json perspective, and this patch throws JString into the mixture further exacerbating the issue.

also, the fix to this issue should ideally be backported to 1.2.x since the issue is present in 1.2.10 also - in some projects, this can turn into a security bug leading to remote exploits (since json data often comes from untrusted sources).

@timotheecour
Copy link
Member Author

timotheecour commented Mar 16, 2021

though it's worth noting that it breaks backwards compatibility with earlier nim versions.

IMO it's fine in this case:

  • 1.4 was raising, now it's giving correct results
  • 1.2 and 1.0 were giving incorrect result (eg: $%(uint.high) was
    -1), now it's giving correct results (18446744073709551615); code that relies on bugs will eventually break

why is the isUnquoted flag not inside the JString case?

this can be changed in future work, if needed; note that sizeof(JsonNodeObj) remains 40 with or without isUnquoted on 64bit (haven't checked on 32bit). The biggest offender is fields*: OrderedTable[string, JsonNode] which makes it 40 instead of 16 (even with isUnquoted as non-variant field). There should be a better way (out of scope for this PR though), even without packedjson

why is it not public? this is significant information when parsing some json files.

not making it public means we can change implementation, and in fact #15776 might very well be a more efficient implementation, see #15768 (comment)

we should however add an API proc jsonKind(self: JsonNode): JsonKind, such that:

  import std/json
  echo uint.high
  let j = parseJson("184467440737095516159999999")
  echo j.kind # JString
  echo j.jsonKind # Number

in general, json is hard to use because it's an odd juxtaposition of the json standard and nim - in json, there are no integers, only numbers which makes JInt vs JFloat distinction unnatural to work with, from a json perspective, and this patch throws JString into the mixture further exacerbating the issue.

see above, jsonKind would solve this.

=> see RFC nim-lang/RFCs#353

also, the fix to this issue should ideally be backported to 1.2.x since the issue is present in 1.2.10 also - in some projects, this can turn into a security bug leading to remote exploits (since json data often comes from untrusted sources).

this opens a can of worms, because it'd also require backporting #15768; people should upgrade eventually or be ok living with some bugs

@arnetheduck
Copy link
Contributor

IMO it's fine in this case:

we have users depending on this particular behavior and need to notify them in turn - providing a changelog and documenting the breaking behavior is just common sense, regardless if you consider the changed/original behavior buggy or not - in the end it breaks actual software out there for actual users.

@arnetheduck
Copy link
Contributor

upgrade eventually

this would be a lot easier if the json module, and similar large modules that have no relationship to the language and compiler, were published outside the standard library - as is, upgrading nim, even between point versions, brings in a host of undesired changes that break backwards compatibility.

timotheecour added a commit to timotheecour/Nim that referenced this pull request Mar 16, 2021
@timotheecour
Copy link
Member Author

timotheecour commented Mar 16, 2021

we have users depending on this particular behavior
providing a changelog and documenting the breaking behavior

adding changelog in #17400
note that the real breaking change was in 1.2=>1.4 because 1.2 gave -1 and 1.4 gave a Defect

this would be a lot easier if the json module, and similar large modules that have no relationship to the language and compiler, were published outside the standard library - as is, upgrading nim, even between point versions, brings in a host of undesired changes that break backwards compatibility.

whether or not this would be desirable, this is not as simple as what you describe.

  • Both the compiler and stdlib depend on std/json and other such modules, in several places. How would you handle that, including bootstrap and circular dependencies between compiler, stdlib and nimble?
  • this would have similar issues as fusion (see Fusion and stdlib evolution RFCs#310)

This means even though Fusion is supposed to be a staging area for the stdlib, contributing to Nim's stdlib is easier than contributing to Fusion. Clearly against Fusion's design

Also, you've made the same comment recently, see my previous answer here: status-im/nim-libbacktrace#12 (comment) which has more details on this (dmd vs phobos split causing friction, monorepo benefits etc).

Instead of scattering the discussion in review comments, please create a dedicated issue / RFC to discuss what you have in mind.

@timotheecour timotheecour deleted the pr_fix_17383_json_uint64 branch March 16, 2021 22:40
timotheecour added a commit that referenced this pull request Mar 17, 2021

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
Co-authored-by: flywind <xzsflywind@gmail.com>
narimiran pushed a commit that referenced this pull request Mar 18, 2021
…nt|uint64 (#17389) [backport:1.2]

* fix #17383: json.%,to and jsonutils.formJson,toJson now works with uint|uint64
* fixup
* fix for js

(cherry picked from commit 895a40d)
@arnetheduck
Copy link
Contributor

So, to provide some background - the json module was broken by #15237 which introduces a backwards-incompatible language change - introducing such patches generally requires reviewing the standard library for code that breaks, such as json - it's quite certain that there are other modules in Nim broken by this.

The fix was later backported to 1.2 - we're now trying to upgrade to 1.2.10, and are running into these bugs - they are significant from our point of view because they introduce remote exploits and require manually reviewing all conversions across the codebase - this is why, at minimum, a changelog entry should be natural to have - changes like these have implications for users of the language - I appreciate #17400!

Regarding modules, I'm pointing it out in this issue that having modules standalone is easier for downstream users like us because it's such a practical and obvious case where having things in the standard library makes things hard - it can serve as background information in a future RFC - if json is used in the compiler, the compiler would simply depend on a json package that it pulls in as part of its build - or keep its own private copy for things it needs - this would also solve issues around bootstrapping where the standard library and compiler-used-to-compile-itself - same issue as csources in general.

@timotheecour
Copy link
Member Author

the json module was broken by #15237 which introduces a backwards-incompatible language change

it's often impossible to fix bugs without introducing some kind of regression, when users depend on prior behavior (which is why the 1.0 stability promise should be IMO taken with a grain of salt, as enforcing stability would prevent much needed bug fixes and improving the language); the patch vs minor distinction is useful as a general guideline but comes with this disclaimer.

But yes, #15237 could've come with a changelog entry and could still be added retroactively (past changelogs are now version controlled)

if json is used in the compiler, the compiler would simply depend on a json package that it pulls in as part of its build - or keep its own private copy for things it needs

related: please read this thread nim-lang/fusion#22

narimiran pushed a commit that referenced this pull request Mar 18, 2021
…nt|uint64 (#17389) [backport:1.2]

* fix #17383: json.%,to and jsonutils.formJson,toJson now works with uint|uint64
* fixup
* fix for js

(cherry picked from commit 895a40d)
narimiran pushed a commit that referenced this pull request Mar 19, 2021
…nt|uint64 (#17389) [backport:1.2]

* fix #17383: json.%,to and jsonutils.formJson,toJson now works with uint|uint64
* fixup
* fix for js

(cherry picked from commit 895a40d)
@narimiran
Copy link
Member

I'm having a very hard time to backport this to 1.2.x because it relies on stuff introduced later (e.g., but not limited to, newJRawNumber, introduced in #15768).

@timotheecour Could you maybe make a variant of this PR catered specifically for 1.2.x, using stuff only available there, i.e. based on version-1-2 branch?

@timotheecour
Copy link
Member Author

I don't think this should be backported, see note above: #17389 (comment)

@arnetheduck
Copy link
Contributor

I don't think this should be backported

the reasonable thing for a backport would be to keep returning -1 probably (using cast[..]) - ie that's the observable behavior that 1.2 has, which probably shouldn't be changed in a point release.,

ringabout pushed a commit to ringabout/Nim that referenced this pull request Mar 22, 2021
… with uint|uint64 (nim-lang#17389) [backport:1.2]

* fix nim-lang#17383: json.%,to and jsonutils.formJson,toJson now works with uint|uint64
* fixup
* fix for js
ringabout added a commit to ringabout/Nim that referenced this pull request Mar 22, 2021
Co-authored-by: flywind <xzsflywind@gmail.com>
@narimiran
Copy link
Member

So, to provide some background - the json module was broken by #15237 which introduces a backwards-incompatible language change

@arnetheduck How about we revert #15237 in the 1.2.x branch, so you get the old behaviour back in 1.2.12?

@narimiran narimiran mentioned this pull request Mar 22, 2021
@arnetheduck
Copy link
Contributor

@narimiran sgtm, it's a prudent solution given that other places in the standard library might have changed behavior as a side effect of that change - we're in the process of auditing our code base to deal with the new Defect but it's quite difficult since aliases, borrows and distinct types are affected as well which makes a trivial "search-and-replace" hard - the same applies to the standard library in general.

ardek66 pushed a commit to ardek66/Nim that referenced this pull request Mar 26, 2021
… with uint|uint64 (nim-lang#17389) [backport:1.2]

* fix nim-lang#17383: json.%,to and jsonutils.formJson,toJson now works with uint|uint64
* fixup
* fix for js
ardek66 pushed a commit to ardek66/Nim that referenced this pull request Mar 26, 2021
Co-authored-by: flywind <xzsflywind@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

json.% raises Defect for uint64
4 participants